fix(input): preserve native key lifecycle across routing - #2142
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThe change introduces protocol 19 input metadata, native Windows key-record handling, lease-based press/repeat/release routing, direct text commits, ConPTY fallback encoding, and expanded Windows and transport tests. ChangesWindows input lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsConsole
participant ClientInput
participant WireProtocol
participant App
participant InputLeaseTable
participant PaneRuntime
WindowsConsole->>ClientInput: Emit native key record
ClientInput->>WireProtocol: Encode key, source, repeat count, and text
WireProtocol->>App: Deliver structured input event
App->>InputLeaseTable: Normalize press and plan repeat
InputLeaseTable->>PaneRuntime: Forward press, repeat, or release to original pane
PaneRuntime-->>App: Report delivery or runtime loss
App->>InputLeaseTable: Remove lease after release or shutdown
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
the
ai-review
Greptile SummaryThe PR introduces a canonical input-event model and lease-based key lifecycle routing.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/app/input/lease.rs | Introduces the lease state machine that preserves ownership and repeat behavior across the complete key lifecycle. |
| src/app/runtime.rs | Routes local key, text, focus, repeat, and release events through the new lease and committed-text paths. |
| src/protocol/wire.rs | Bumps protocol 19 and extends structured client input with repeat counts, generated text, text commits, and source provenance. |
| src/client/input/windows_vti.rs | Preserves native Windows input records while retaining semantic handling for synthesized and VT input. |
| src/app/input/mod.rs | Adds dedicated committed-text handling while retaining the existing headless backpressure policy discussed in the withdrawn prior thread. |
| src/pane/terminal.rs | Selects native Windows record delivery only at compatible terminal destinations and otherwise uses semantic encoding. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Client input] --> B{Input kind}
B -->|Key| C[Canonical TerminalKey]
B -->|Committed text| D[TextCommit path]
B -->|Paste or mouse| E[Existing semantic path]
C --> F[Input lease state machine]
F -->|Consumed| G[Suppress or reprocess repeats]
F -->|Forwarded| H[Retain destination ownership]
H --> I{Destination supports native record?}
I -->|Yes| J[ConPTY native console record]
I -->|No| K[Semantic terminal encoding]
D --> L[Active modal or terminal target]
Reviews (4): Last reviewed commit: "fix(input): preserve native key lifecycl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/server/headless.rs (1)
4547-4547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for background text promotion.
RawInputEvent::Text(_)now promotes its source client to the foreground. The supplied tests cover key, focus, and mouse promotion, but do not cover this new branch. Add a test that sendsClientInputEvent::TextCommitfrom a background client and verifies foreground promotion and delivery to the focused target.src/server/client_transport.rs (1)
1605-1641: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for mixed paste + non-paste overflow classification.
The new tests cover pure repeat-count overflow,
generated_textoverflow, andTextCommitoverflow, but none exercises a batch that mixes an oversizedPastewith aKey/TextCommitevent. Given the classification hinges oninput_bytes == 0(see Line 434 comment), add a test that locks in the intended behavior for this mixed case.src/protocol/wire.rs (1)
1103-1157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the frozen byte vector to help future protocol changes.
The exact encoded byte assertion freezes the protocol 19 input envelope. The comment states the intent, but the vector alone does not show which field produced each byte group. Add a short field-order note so a later field addition is easy to re-derive.
src/raw_input.rs (1)
68-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider attaching the flushed bytes to the text-commit key.
extract_one_eventat Lines 768-770 attacheswith_vt_bytesto plain UTF-8 keys. This flush path calls onlywith_text_commit(), so the key keepsKeySource::Synthesized. The range record still reportsstartandlen, so no behavior breaks today. Attaching the bytes keeps provenance consistent across both paths.♻️ Proposed change
- event: RawInputEvent::Key(key.with_text_commit()), + event: RawInputEvent::Key( + key.with_text_commit().with_vt_bytes(buffer.clone()), + ),scripts/windows_conpty_enhanced_input_probe.ps1 (1)
135-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
Send-NativeRecordAndObserveobserve only new output.The helper searches the whole pane buffer for the needle. It does not snapshot the pane text before sending. The two current call sites use distinct expected records, so they cannot alias today. If a later check sends a record that a previous check already produced,
Wait-PaneTextreturns immediately on the stale line and reports a false pass.
Send-KeyAndObserveandSend-RawAndObservealready avoid this by capturing a$beforevalue. Count occurrences and require an increase.♻️ Proposed refactor to require a new occurrence
function Send-NativeRecordAndObserve { param( [string] $PaneId, [string] $Text, [string] $ExpectedRecord ) + $needle = "PROBE_RECORD:$ExpectedRecord" + $before = ([regex]::Matches((Read-Pane -PaneId $PaneId), [regex]::Escape($needle))).Count & $script:Exe pane send-text $PaneId $Text | Out-Null if ($LASTEXITCODE -ne 0) { throw "pane send-text failed with exit code $LASTEXITCODE" } - $needle = "PROBE_RECORD:$ExpectedRecord" - $paneText = Wait-PaneText -PaneId $PaneId -Needle $needle + $deadline = (Get-Date).AddSeconds(10) + do { + $paneText = Read-Pane -PaneId $PaneId + $after = ([regex]::Matches($paneText, [regex]::Escape($needle))).Count + if ($after -gt $before) { + break + } + Start-Sleep -Milliseconds 200 + } while ((Get-Date) -lt $deadline) return [ordered]@{ expected_record = $needle - delivered = $paneText.Contains($needle) + delivered = $after -gt $before pane = $paneText } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 24cbd854-178b-46cb-aa41-ba5a6b5a1097
📒 Files selected for processing (36)
docs/next/CHANGELOG.mddocs/next/api/herdr-api.schema.jsonscripts/windows_check.ps1scripts/windows_conpty_enhanced_input_probe.ps1src/app/api/panes.rssrc/app/input/clipboard.rssrc/app/input/copy_mode.rssrc/app/input/lease.rssrc/app/input/mod.rssrc/app/input/modal.rssrc/app/input/navigate.rssrc/app/input/terminal.rssrc/app/mod.rssrc/app/popup.rssrc/app/runtime.rssrc/app/state.rssrc/app/worktrees.rssrc/client/input.rssrc/client/input/windows_vti.rssrc/client/mod.rssrc/config/keybinds.rssrc/input/encode.rssrc/input/mod.rssrc/input/model.rssrc/input/parse.rssrc/pane/input.rssrc/pane/terminal.rssrc/platform/windows.rssrc/protocol/wire.rssrc/raw_input.rssrc/server/client_transport.rssrc/server/clients.rssrc/server/headless.rstests/api_ping.rstests/cli/sessions.rstests/support/mod.rs
ebc9dc3 to
877f7fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9fd4ced-8d7f-474a-97de-22ff1130f530
📒 Files selected for processing (36)
docs/next/CHANGELOG.mddocs/next/api/herdr-api.schema.jsonscripts/windows_check.ps1scripts/windows_conpty_enhanced_input_probe.ps1src/app/api/panes.rssrc/app/input/clipboard.rssrc/app/input/copy_mode.rssrc/app/input/lease.rssrc/app/input/mod.rssrc/app/input/modal.rssrc/app/input/navigate.rssrc/app/input/terminal.rssrc/app/mod.rssrc/app/popup.rssrc/app/runtime.rssrc/app/state.rssrc/app/worktrees.rssrc/client/input.rssrc/client/input/windows_vti.rssrc/client/mod.rssrc/config/keybinds.rssrc/input/encode.rssrc/input/mod.rssrc/input/model.rssrc/input/parse.rssrc/pane/input.rssrc/pane/terminal.rssrc/platform/windows.rssrc/protocol/wire.rssrc/raw_input.rssrc/server/client_transport.rssrc/server/clients.rssrc/server/headless.rstests/api_ping.rstests/cli/sessions.rstests/support/mod.rs
🚧 Files skipped from review as they are similar to previous changes (34)
- src/app/api/panes.rs
- docs/next/api/herdr-api.schema.json
- src/client/mod.rs
- src/app/input/copy_mode.rs
- src/app/popup.rs
- tests/api_ping.rs
- scripts/windows_check.ps1
- src/input/encode.rs
- src/input/parse.rs
- src/pane/input.rs
- tests/support/mod.rs
- src/app/runtime.rs
- tests/cli/sessions.rs
- scripts/windows_conpty_enhanced_input_probe.ps1
- src/app/input/clipboard.rs
- src/server/clients.rs
- src/app/input/modal.rs
- src/app/worktrees.rs
- src/platform/windows.rs
- src/input/mod.rs
- src/server/client_transport.rs
- src/app/state.rs
- src/app/input/lease.rs
- src/server/headless.rs
- src/client/input.rs
- src/app/input/mod.rs
- src/input/model.rs
- src/pane/terminal.rs
- docs/next/CHANGELOG.md
- src/config/keybinds.rs
- src/app/input/terminal.rs
- src/app/input/navigate.rs
- src/client/input/windows_vti.rs
- src/app/mod.rs
877f7fb to
8599943
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/input/model.rs (1)
184-191: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
is_ascii_uppercasedrops text commits for non-ASCII uppercase characters.A key such as
Char('É')withKeyModifiers::SHIFTfalls into the second arm, which requires empty modifiers. The text commit is then skipped for that keystroke. Usechar::is_uppercaseto cover non-ASCII layouts.♻️ Proposed change
- KeyCode::Char(ch) if ch.is_ascii_uppercase() => { + KeyCode::Char(ch) if ch.is_uppercase() => { self.modifiers == KeyModifiers::SHIFT || self.modifiers.is_empty() }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f8bc1bb-48b8-4882-827c-72f37cc286e7
📒 Files selected for processing (36)
docs/next/CHANGELOG.mddocs/next/api/herdr-api.schema.jsonscripts/windows_check.ps1scripts/windows_conpty_enhanced_input_probe.ps1src/app/api/panes.rssrc/app/input/clipboard.rssrc/app/input/copy_mode.rssrc/app/input/lease.rssrc/app/input/mod.rssrc/app/input/modal.rssrc/app/input/navigate.rssrc/app/input/terminal.rssrc/app/mod.rssrc/app/popup.rssrc/app/runtime.rssrc/app/state.rssrc/app/worktrees.rssrc/client/input.rssrc/client/input/windows_vti.rssrc/client/mod.rssrc/config/keybinds.rssrc/input/encode.rssrc/input/mod.rssrc/input/model.rssrc/input/parse.rssrc/pane/input.rssrc/pane/terminal.rssrc/platform/windows.rssrc/protocol/wire.rssrc/raw_input.rssrc/server/client_transport.rssrc/server/clients.rssrc/server/headless.rstests/api_ping.rstests/cli/sessions.rstests/support/mod.rs
🚧 Files skipped from review as they are similar to previous changes (35)
- docs/next/CHANGELOG.md
- tests/api_ping.rs
- src/app/api/panes.rs
- src/client/mod.rs
- src/app/input/copy_mode.rs
- tests/cli/sessions.rs
- docs/next/api/herdr-api.schema.json
- src/app/popup.rs
- scripts/windows_check.ps1
- src/input/mod.rs
- src/platform/windows.rs
- src/app/input/mod.rs
- src/input/parse.rs
- src/server/clients.rs
- src/app/input/modal.rs
- src/client/input.rs
- tests/support/mod.rs
- src/server/client_transport.rs
- src/input/encode.rs
- src/pane/input.rs
- src/app/input/clipboard.rs
- src/pane/terminal.rs
- src/server/headless.rs
- src/app/input/terminal.rs
- src/config/keybinds.rs
- src/raw_input.rs
- src/app/runtime.rs
- src/app/worktrees.rs
- src/app/input/navigate.rs
- src/app/state.rs
- src/app/mod.rs
- src/protocol/wire.rs
- src/app/input/lease.rs
- scripts/windows_conpty_enhanced_input_probe.ps1
- src/client/input/windows_vti.rs
refs #2077 Co-authored-by: Jonathan Liebig <jonathan.liebig@gmail.com>
8599943 to
a72702a
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
As requested some Windows tests: Tested exact #2142 head
Edit: Tested the remainders. Only noteworthy issue I can think of is BMP emojis not working consistently. That is an upstream OpenConsole/ConPTY bug though, Microsoft fixed that for Windows Terminal but not yet for ConPTY. So clearly out of scope for herdr imo. |
|
@Pimpmuckl ty for all the help! |
Range b411274..26a7bc8 (7 commits): windows semantic escape taps, native key lifecycle rework (herdrdev#2142), remove experiments settings TUI, bottom tab bar placement (herdrdev#2118), agentmd protocol update, alternate-screen agent history read, packed sidebar worktree groups. Protocol: source PROTOCOL_VERSION is 19 (upstream bumped 18->19 for the incompatible ClientKeySource/TextCommit wire change). Stable published 17, preview published 18; 19 is unpublished and ahead of both channels, so no further bump. Test fixtures already expect protocol 19. Conflict resolutions: - config.rs / app/mod.rs / app/input/mod.rs: additive merges keeping fork features (StatusConfig, command_palette, pub(crate) modal) alongside upstream additions (TabBarPositionConfig, limit_snapshot_lines, lease mod). - app/input/navigate.rs: preserved the fork's revert of non-us shifted keybindings (herdrdev#1876) - kept unmodified_digit_for_key and its call sites removed - while adopting upstream's &TerminalKey lifecycle signatures the merged call chain requires. - config/keybinds.rs: adopted upstream's matched_index (&TerminalKey borrow required by indexed_navigation_action); kept the fork's herdrdev#1876 test deletions removed. - app/input/mouse.rs: kept both fork tab-click tests and upstream's bottom-mode-bar test. - ui.rs auto-merged: fork status_right strip coexists with upstream bottom tab bar placement.
3 participants
Summary
modifyOtherKeys, API input, and non-Windows encoding on semantic pathsWindows exposed the failure through Escape in #2077, but the underlying problem was shared input routing: Herdr mixed semantic keys, committed text, physical provenance, repeat lifecycle, and destination encoding in one representation. This separates those concerns instead of adding another Windows-only key workaround.
This supersedes #2081. The final implementation was built from Jonathan Liebig's original Windows work and credits him as co-author, then restructures it around the shared input model and lease state machine.
Refs #2077.
Validation
just check: 3,138 Rust tests, clippy, Windows cross-clippy, integration tests, and 93 maintenance testsReadConsoleInputWConPTY probe covering grouped Escape press/repeat/release recordsBoundaries
This does not change raw
pane send-textEscape behavior from #1929, Ctrl+Break signaling, terminal emulation, rendering, or pane layout. Protocol 19 remains required because protocol 18 shipped in preview.